-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[test] baseline forge #15586
[test] baseline forge #15586
Conversation
⏱️ 2h 4m total CI duration on this PR
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
) | ||
.await | ||
.unwrap(); | ||
panic!("test_fault_tolerance_of_leader_equivocation"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The panic!()
statement at the end of this test appears to be forcing a failure, which defeats the purpose of the test's actual validation logic through successful_criteria()
. Since the test already has proper assertions, this line should be removed to allow the test to properly validate the leader equivocation fault tolerance behavior.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
335ddba
to
d258c80
Compare
impl NetworkLoadTest for PerformanceBenchmark { | ||
async fn test( | ||
&self, | ||
swarm: Arc<tokio::sync::RwLock<Box<dyn Swarm>>>, | ||
_report: &mut TestReport, | ||
duration: Duration, | ||
) -> Result<()> { | ||
let validators = { swarm.read().await.get_validator_clients_with_names() }; | ||
let num_bad_leaders = validators.len() / 10; | ||
for (index, (name, validator)) in validators.iter().enumerate().take(num_bad_leaders) { | ||
validator | ||
.set_failpoint( | ||
"consensus::leader_equivocation".to_string(), | ||
"off".to_string(), | ||
) | ||
.await | ||
.map_err(|e| { | ||
anyhow!( | ||
"set_failpoint to set consensus leader equivocation on {} failed, {:?}", | ||
name, | ||
e | ||
) | ||
})?; | ||
}; | ||
Ok(()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test currently disables leader equivocation but returns immediately without waiting for the specified test duration. Adding tokio::time::sleep(duration).await
before returning would allow the test to run for its full intended duration and properly observe the system behavior with the disabled failpoint.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
d258c80
to
20a64ae
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
76e5434
to
6cd0fc7
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6cd0fc7
to
829eac0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
46c8d80
to
eaee735
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c511596
to
667809b
Compare
667809b
to
bbe737d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This reverts commit f3df9fa.
This comment has been minimized.
This comment has been minimized.
❌ Forge suite
|
a492fa9
to
cc7eeb5
Compare
Description
How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist